I think it's reasonable to start accumulating common driver interfaces in the core jumpstarter, as, for example, this one is broadly used around Jumpstarter, and lived on opendal initially.

Agreed! The FlasherInterface ABC is purely abstract with no external dependencies, so it makes sense for it to live in core. This way any driver can implement it without pulling in opendal as a transitive dependency.

Are we testing all paths in here: https://github.com/jumpstarter-dev/jumpstarter/blob/main/python/packages/jumpstarter-driver-qemu/jumpstarter_driver_qemu/driver_test.py#L79 ?

I see it uses the Operator, can we make sure that we take that and interpret that for compatibility purposes without really relying/using opendal?

Good question. The current test covers two paths:

1. Local file path (cached_image.resolve()) -- this exercises the _local_file_adapter code path in the new FlasherClient, streaming the file via resource_async.
2. HTTP URL (the Fedora download URL) -- this was previously using Operator("http", endpoint=...) and is now a direct https:// URL, which exercises the _http_url_adapter code path using PresignedRequestResource.

Both paths in the new FlasherClient are covered by the integration test.

Regarding backward compatibility with opendal.Operator: the opendal package's FlasherClient (in jumpstarter_driver_opendal.client) is unchanged and still accepts the operator= parameter. Drivers that depend on jumpstarter-driver-opendal (esp32, pi-pico, dutlink, sdwire) continue to use that client via their own FlasherInterface.client() classmethod which points to the opendal version.

The QEMU driver now uses the core FlasherInterface which points to the simplified FlasherClient -- this one intentionally does not accept an operator= parameter since the goal is to avoid the opendal dependency entirely. The Operator usage in the old test was only needed because the old flasher client required it for HTTP downloads; the new client handles HTTP URLs natively.

So the API surface for QEMU users changes from:

# Old (required opendal)
qemu.flasher.flash("path/on/server", operator=Operator("http", endpoint="https://example.com"))

# New (no opendal needed)
qemu.flasher.flash("https://example.com/path/on/server")

The new API is simpler and doesn't break any existing QEMU usage since the operator= parameter was only used in this test, not in production QEMU workflows.

Ok, but if that test is there, is not executing. Should we make sure to have a test that executes the full url download?

You're right -- when the cached image exists (which it does in CI), the HTTP URL branch is never exercised.

I'll add a dedicated unit test for the _http_url_adapter and _parse_path helpers in the core jumpstarter package to make sure the URL code path is covered without needing a full Fedora image download. This will verify that:

1. _parse_path correctly distinguishes local paths from HTTP/HTTPS URLs
2. _http_url_adapter produces a proper PresignedRequestResource for HTTP URLs
3. The FlasherClient._flash_single method routes HTTP URLs through the presigned request path

For a full end-to-end HTTP download test, that would involve downloading a large image in CI which is expensive. I think a unit test covering the routing logic is the right trade-off here. Does that work for you?

[MEDIUM] Parallel FlasherInterface ABCs with no inheritance relationship

There are now two identical FlasherInterface ABCs -- this one in core and the one in jumpstarter-driver-opendal -- with no inheritance link between them. Both define flash() and dump() with the same signatures but different client() return values. Any future contract change must be manually synchronized.

Since this is phase 1 of the incremental migration (#441), consider having the opendal FlasherInterface inherit from this core one in a follow-up, or plan to remove the opendal copy once remaining drivers migrate.

AI-generated, human reviewed

Agreed -- this duplication is intentional for phase 1 of #441 to keep this PR focused on removing the opendal dependency from QEMU without touching the other drivers.

For the follow-up, I think the cleanest approach is to have the opendal FlasherInterface inherit from this core one (they already have identical flash()/dump() signatures). That way any future contract change only needs to happen in one place. I'll track this in #441.

[MEDIUM] No test coverage for _local_file_adapter streaming behavior

The tests cover path parsing and routing logic via mocks, but the actual _local_file_adapter streaming pipeline is never exercised directly. _AsyncIteratorStream and _FileWriteObjectStream contain non-trivial logic (lazy file open, chunked reading, progress attributes) that could break silently. Consider adding at least a focused unit test that exercises _local_file_adapter with a real temp file to verify the streaming and chunking behavior end-to-end.

AI-generated, human reviewed

Good point. The streaming internals (_AsyncIteratorStream, _FileWriteObjectStream, chunked reading) are non-trivial and currently only tested indirectly through the routing mocks.

I'd prefer to add a focused streaming integration test as a follow-up rather than in this PR, since it would need to set up a mock DriverClient with a working resource_async pipeline, which adds meaningful complexity. The routing tests give us confidence the right code paths are selected, and the e2e QEMU tests exercise the full pipeline end-to-end. Will track this as a follow-up to #441.

[MEDIUM] Module-level docstrings may violate project comment policy

This file, driver/flasher.py, and flasher_test.py all contain module-level docstrings explaining the implementation approach. The project rules state comments should be a last resort when code cannot be self-explanatory. Existing comparable modules (e.g., opendal/client.py, opendal/driver.py) do not use module-level docstrings. The module names, class names, and method signatures already convey purpose -- consider removing these docstrings to stay consistent with the rest of the codebase.

AI-generated, human reviewed

Fair point -- removed all three module-level docstrings in 0c2d55c to match the rest of the codebase. The module/class/method names are self-explanatory.

[LOW] _AsyncIteratorStream.aclose() does not propagate close to wrapped async generator

The aclose() method is a no-op, so if the stream is abandoned before the async generator completes (e.g., on error), the file handle inside the generator may not be closed promptly. Adding await self.iterator.aclose() would ensure the async with block in the generator properly closes the file handle. Impact is minor (only affects error paths, GC will eventually clean up), but it would be cleaner.

async def aclose(self):
    await self.iterator.aclose()

AI-generated, human reviewed

Good catch. Fixed in 0c2d55c -- aclose() now calls await self.iterator.aclose() which will properly trigger the async with cleanup in the generator and close the file handle on error paths.

[LOW] No test for dict-based multi-target flash

FlasherClient.flash() supports passing dict[str, PathBuf] for multi-target flashing (around lines 258-267 of flasher.py). No test covers this code path, including the ArgumentError case when both a dict and a target kwarg are provided. Adding a test for the happy path and the error case would improve confidence in this feature.

AI-generated, human reviewed

Agreed this would be good coverage. The dict-based multi-target flash path (including the ArgumentError guard) is straightforward but untested. I'll add tests for both the happy path and the error case as a follow-up -- keeping this PR focused on the core changes to avoid scope creep.

[LOW] No test for dump with local path routing

TestFlasherClientRouting tests dump routing for HTTP URLs but has no corresponding test for dump routing to local file paths. Adding a test_dump_local_path case would round out the routing coverage.

AI-generated, human reviewed

Added test_dump_routes_local_path in 0c2d55c -- this mirrors the existing test_dump_routes_http_url test and verifies that local paths go through _local_file_adapter with the correct mode="wb" parameter.

[MEDIUM] Still not addressed from previous review: The new core FlasherInterface and the existing jumpstarter_driver_opendal.driver.FlasherInterface are parallel ABCs with identical method signatures but no inheritance relationship. This means drivers switching from opendal's FlasherInterface to the core one (like QEMU did) are making a silent, potentially breaking change for any code doing isinstance checks. The opendal FlasherInterface should be updated to inherit from the core one, or at minimum re-export it, to establish a clear hierarchy.

AI-generated, human reviewed

[MEDIUM] Still not addressed from previous review: _AsyncIteratorStream and _FileWriteObjectStream have no direct unit tests exercising their streaming behavior (receive/send/aclose lifecycle, error propagation on partial reads, EndOfStream on empty files, etc.). The existing routing tests mock these classes entirely, so actual I/O paths remain uncovered.

AI-generated, human reviewed

[LOW] Still not addressed from previous review: No test for dict-based multi-target flash. FlasherClient.flash accepts a dict[str, PathBuf] and iterates over entries calling _flash_single for each, and also raises ArgumentError when both a dict and target are provided. Neither path is tested.

AI-generated, human reviewed

[MEDIUM] The compression parameter is silently ignored when the path is an HTTP URL. _http_url_adapter does not accept a compression keyword, so _flash_single drops it without warning when routing to the HTTP path. The same applies to dump at lines 273-275. Either raise an error/warning when compression is specified with an HTTP URL, or pass it through to the exporter.

AI-generated, human reviewed

[LOW] Parameter naming is inconsistent between the driver-side FlasherInterface and the client-side FlasherClient. The driver's dump(self, target, partition) uses target for the destination resource and partition for the optional selector, while this client's dump(self, path, *, target) uses path for the destination and target for the selector. Since the client maps its target kwarg to the driver's partition positionally via self.call("dump", handle, target), this works at runtime but creates a confusing API surface. Consider aligning the naming.

AI-generated, human reviewed